Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rubocop: Discourage the use of rm_f and rm_rf in formulae and casks #17705

Merged
merged 10 commits into from
Aug 1, 2024

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Jul 13, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

  • This cop checks for the use of FileUtils.rm_rf and suggests using FileUtils.rm_r because we should know if we couldn't delete a thing for some reason, not just force it.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! Could we do this for rmtree too?

@Bo98
Copy link
Member

Bo98 commented Jul 13, 2024

Worth noting because we do include FileUtils that formulae will use rm_rf (self/no receiver).

However because we do that we can also replace rm_rf in formulae with something better as we're not really monkey patching anything there - the Formula class is our own API.

@MikeMcQuaid
Copy link
Member

Worth noting because we do include FileUtils that formulae will use rm_rf (self/no receiver).

Yes, good point/catch.

However because we do that we can also replace rm_rf in formulae with something better as we're not really monkey patching anything there - the Formula class is our own API.

I think the existing behaviour of rm_rf makes sense: it's like rm -rf and there are cases (although few in Homebrew) where you do want to avoid a failure when the file doesn't exist. I'd sooner see us use a better call instead.

Point taken, though, that in other cases yeh this is a good case where we should just use our own APIs here.

@Bo98
Copy link
Member

Bo98 commented Jul 13, 2024

Point taken, though, that in other cases yeh this is a good case where we should just use our own APIs here.

Yeah at least we have that for FileUtils in formulae. The annoying one is Pathname#rmtree. The change in behaviour in newer Ruby is surprising and subtle. It's a shame there's not another Pathname method that can be used instead and instead needs to be FileUtils now.

Pretty much all existing rmtree usages are however likely safe to use as FilesUtils.rm_r as they were probably all written when rmtree did use that.

@issyl0 issyl0 force-pushed the rubocop-avoid-fileutils-rmrf branch 2 times, most recently from 900c0f9 to a7a1dbd Compare July 13, 2024 20:35
@issyl0 issyl0 force-pushed the rubocop-avoid-fileutils-rmrf branch 4 times, most recently from 87e90fe to 2e9053b Compare July 14, 2024 03:27
@issyl0
Copy link
Member Author

issyl0 commented Jul 14, 2024

Hmmm, autofixing stuff makes all of the tests fail. 🤔

@MikeMcQuaid
Copy link
Member

@issyl0 I think this could be scoped just to formulae/casks, if that made things easier?

@issyl0
Copy link
Member Author

issyl0 commented Jul 14, 2024

In formulae and casks, now:

7091 files inspected, 282 offenses detected, 282 offenses autocorrectable

Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

Library/Homebrew/rubocops/no_fileutils_rmrf.rb Outdated Show resolved Hide resolved
@issyl0
Copy link
Member Author

issyl0 commented Jul 15, 2024

There appears to be a lack of consensus on the formula fixes PR as to if this is the right approach. Anyone else got any opinions there?

@issyl0 issyl0 requested a review from Bo98 July 15, 2024 18:14
@issyl0 issyl0 changed the title rubocop: Discourage the use of FileUtils.rm_rf rubocop: Discourage the use of rm_f and rm_rf in formulae and casks Jul 24, 2024
@issyl0 issyl0 force-pushed the rubocop-avoid-fileutils-rmrf branch from 53c2c85 to 16a759d Compare July 24, 2024 22:35
@issyl0 issyl0 force-pushed the rubocop-avoid-fileutils-rmrf branch from 25f8a84 to 0872966 Compare August 1, 2024 17:29
@issyl0
Copy link
Member Author

issyl0 commented Aug 1, 2024

External link https://marketplace.visualstudio.com/items?itemName=sharat.vscode-brewfile failed (status code 503) Yeah, this part of Azure is down it seems.

@issyl0 issyl0 enabled auto-merge August 1, 2024 17:38
@issyl0 issyl0 merged commit 53e3632 into master Aug 1, 2024
24 checks passed
@issyl0 issyl0 deleted the rubocop-avoid-fileutils-rmrf branch August 1, 2024 17:42
issyl0 added a commit that referenced this pull request Aug 4, 2024
- After all the work that went into
  #17705, we
  don't want the docs disagreeing with what CI says
  the style should be.
issyl0 added a commit that referenced this pull request Aug 4, 2024
- After all the work that went into
  #17705, we
  don't want the docs disagreeing with what CI says
  the style should be.
ctaintor pushed a commit to ctaintor/brew that referenced this pull request Sep 4, 2024
- After all the work that went into
  Homebrew#17705, we
  don't want the docs disagreeing with what CI says
  the style should be.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants